-
Notifications
You must be signed in to change notification settings - Fork 667
CONSOLE-5041: Replace testHook with renderHook from RTL #15936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@logonoff: This pull request references CONSOLE-5041 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/label px-approved changes only affect unit tests: |
|
@logonoff: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@logonoff: This pull request references CONSOLE-5041 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request systematically replaces a deprecated custom testing utility ( 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
frontend/packages/dev-console/src/utils/__tests__/useAddActionExtensions.spec.ts (3)
75-79: Move assertions outside the renderHook callback.The PR description explicitly mentions "Fixes tests that had assertions inside hook callbacks to properly use result.current," but these tests still have assertions inside the callback. This is an anti-pattern—the callback should only render the hook, while assertions should operate on
result.current.Suggested fix
- renderHook(() => { - const [addActionExtensions, resolved] = useAddActionExtensions(); - expect(addActionExtensions).toEqual([addAction1, addAction2, addAction3, addAction4]); - expect(resolved).toEqual(true); - }); + const { result } = renderHook(() => useAddActionExtensions()); + const [addActionExtensions, resolved] = result.current; + expect(addActionExtensions).toEqual([addAction1, addAction2, addAction3, addAction4]); + expect(resolved).toEqual(true);
89-93: Same pattern issue—assertions belong outside the callback.Suggested fix
- renderHook(() => { - const [addActionExtensions, resolved] = useAddActionExtensions(); - expect(addActionExtensions).toEqual([addAction1, addAction2, addAction3, addAction4]); - expect(resolved).toEqual(true); - }); + const { result } = renderHook(() => useAddActionExtensions()); + const [addActionExtensions, resolved] = result.current; + expect(addActionExtensions).toEqual([addAction1, addAction2, addAction3, addAction4]); + expect(resolved).toEqual(true);
103-107: Same pattern issue—assertions belong outside the callback.Suggested fix
- renderHook(() => { - const [addActionExtensions, resolved] = useAddActionExtensions(); - expect(addActionExtensions).toEqual([addAction1, addAction3, addAction4]); - expect(resolved).toEqual(true); - }); + const { result } = renderHook(() => useAddActionExtensions()); + const [addActionExtensions, resolved] = result.current; + expect(addActionExtensions).toEqual([addAction1, addAction3, addAction4]); + expect(resolved).toEqual(true);frontend/packages/dev-console/src/utils/__tests__/usePerspectiveDetection.spec.ts (2)
16-21: Assertions should be outside the renderHook callback.Same issue as the other files—the expect calls should operate on
result.currentafterrenderHookreturns, not inside the callback function.Suggested fix
- renderHook(() => { - const [enablePerspective, loading] = usePerspectiveDetection(); - - expect(enablePerspective).toBe(true); - expect(loading).toBe(true); - }); + const { result } = renderHook(() => usePerspectiveDetection()); + const [enablePerspective, loading] = result.current; + + expect(enablePerspective).toBe(true); + expect(loading).toBe(true);
29-34: Same pattern issue—move assertions outside.Suggested fix
- renderHook(() => { - const [enablePerspective, loading] = usePerspectiveDetection(); - - expect(enablePerspective).toBe(true); - expect(loading).toBe(false); - }); + const { result } = renderHook(() => usePerspectiveDetection()); + const [enablePerspective, loading] = result.current; + + expect(enablePerspective).toBe(true); + expect(loading).toBe(false);frontend/packages/webterminal-plugin/src/components/cloud-shell/__tests__/useActivityTick.spec.ts (1)
24-54: Refactor to use result.current for the tick function.This test has significant logic inside the
renderHookcallback. While the time-manipulation testing approach is valid, the assertions andtick()calls should happen outside the callback usingresult.current.Suggested refactor
- renderHook(() => { - const tick = useActivityTick('testName', 'testNamespace'); - - // send initial tick - tick(); - expect(sendActivityTickMock).toHaveBeenCalledWith('testName', 'testNamespace'); - sendActivityTickMock.mockReset(); - - // elapsed time isn't long enough to trigger sending a new tick - mockNow += 59000; - tick(); - - // reset mock to test count - expect(sendActivityTickMock).toHaveBeenCalledTimes(0); - - // elapsed time is now long enough to trigger sending a tick - mockNow += 1000; - tick(); - expect(sendActivityTickMock).toHaveBeenCalledWith('testName', 'testNamespace'); - expect(sendActivityTickMock).toHaveBeenCalledTimes(1); - - // multiple ticks within the time interval does not result sending tick - tick(); - tick(); - expect(sendActivityTickMock).toHaveBeenCalledTimes(1); - - // advance time enough to send a tick - mockNow += 600000; - tick(); - expect(sendActivityTickMock).toHaveBeenCalledTimes(2); - }); + const { result } = renderHook(() => useActivityTick('testName', 'testNamespace')); + const tick = result.current; + + // send initial tick + tick(); + expect(sendActivityTickMock).toHaveBeenCalledWith('testName', 'testNamespace'); + sendActivityTickMock.mockReset(); + + // elapsed time isn't long enough to trigger sending a new tick + mockNow += 59000; + tick(); + + // reset mock to test count + expect(sendActivityTickMock).toHaveBeenCalledTimes(0); + + // elapsed time is now long enough to trigger sending a tick + mockNow += 1000; + tick(); + expect(sendActivityTickMock).toHaveBeenCalledWith('testName', 'testNamespace'); + expect(sendActivityTickMock).toHaveBeenCalledTimes(1); + + // multiple ticks within the time interval does not result sending tick + tick(); + tick(); + expect(sendActivityTickMock).toHaveBeenCalledTimes(1); + + // advance time enough to send a tick + mockNow += 600000; + tick(); + expect(sendActivityTickMock).toHaveBeenCalledTimes(2);
🧹 Nitpick comments (6)
frontend/packages/webterminal-plugin/src/components/cloud-shell/__tests__/useCloudShellAvailable.spec.ts (1)
32-51: Tests will pass, but thererender()call is unnecessary for these async assertions.The initial
renderHook()already triggersuseEffect, which callscheckTerminalAvailable(). Theawait act(async () => {...})wrapper flushes the microtask queue, allowing the mocked promise to settle. Thererender()inside doesn't contribute to waiting for the async operation.A more idiomatic RTL pattern:
const { result } = renderHook(() => useCloudShellAvailable()); await act(async () => { // Let microtasks flush (promise resolution/rejection) }); expect(result.current).toBe(false);Or using
waitForfor explicit state change detection:import { renderHook, waitFor } from '@testing-library/react'; // ... const { result } = renderHook(() => useCloudShellAvailable()); await waitFor(() => { expect(result.current).toBe(true); });Given this is a bulk migration PR, the current approach is functional and maintains consistency across files. Consider revisiting as a follow-up if you want tighter alignment with RTL best practices.
frontend/packages/console-shared/src/utils/__tests__/pod-ring-utils.spec.ts (1)
214-228: Async API call tests may not await completion.These tests exercise code paths where
checkPodEditAccessis invoked asynchronously (per the hook implementation inpod-ring-utils.ts). The assertions fire immediately afterrenderHookwithout usingwaitFororact. They pass because the initial state (false) happens to match the expected final state, but they don't actually verify that the async operation completed.Consider using
waitForto ensure the hook's effect completes:♻️ Suggested pattern using waitFor
+import { renderHook, waitFor } from '@testing-library/react'; ... - it('should return false when api call returns false for a resource', () => { + it('should return false when api call returns false for a resource', async () => { obj.kind = 'DeploymentConfig'; const { result } = renderHook(() => usePodScalingAccessStatus(obj, DeploymentConfigModel, [], true), ); + await waitFor(() => { + expect(checkPodEditAccessMock).toHaveBeenCalled(); + }); expect(result.current).toBe(false); });This ensures the mock was actually invoked and the async path was exercised, improving test reliability.
frontend/packages/topology/src/filters/__tests__/useSearchFilter.spec.ts (2)
34-41: Assertions insiderenderHookcallback is an anti-pattern.The assertions are placed inside the
renderHookcallback rather than usingresult.currentoutside. While this works because the helpertestUseSearchFilterinvokes the hook synchronously, this pattern couples test assertions to the render phase and deviates from RTL's intended usage.The idiomatic pattern would restructure tests to invoke
testUseSearchFilterviarenderHookand then assert onresult.current:♻️ Suggested refactor for idiomatic usage
it('should handle null | undefined text', () => { - renderHook(() => { - expect(testUseSearchFilter(null, 'test')[0]).toBe(false); - expect(testUseSearchFilter(null, '')[0]).toBe(false); - expect(testUseSearchFilter(null, undefined)[0]).toBe(false); - expect(testUseSearchFilter(undefined, 'test')[0]).toBe(false); - expect(testUseSearchFilter(undefined, '')[0]).toBe(false); - expect(testUseSearchFilter(undefined, undefined)[0]).toBe(false); - }); + expect(renderHook(() => testUseSearchFilter(null, 'test')).result.current[0]).toBe(false); + expect(renderHook(() => testUseSearchFilter(null, '')).result.current[0]).toBe(false); + expect(renderHook(() => testUseSearchFilter(null, undefined)).result.current[0]).toBe(false); + expect(renderHook(() => testUseSearchFilter(undefined, 'test')).result.current[0]).toBe(false); + expect(renderHook(() => testUseSearchFilter(undefined, '')).result.current[0]).toBe(false); + expect(renderHook(() => testUseSearchFilter(undefined, undefined)).result.current[0]).toBe(false); });Alternatively, consider refactoring
testUseSearchFilterto just set up mocks, and have each test callrenderHook(() => useSearchFilter(...))directly for cleaner separation.
44-92: Same pattern issue applies to remaining test cases.All subsequent test cases (lines 44-92) follow the same anti-pattern of placing assertions inside the
renderHookcallback. Consider applying the same refactoring approach consistently across all tests in this file for maintainability.frontend/packages/console-shared/src/hooks/__tests__/usePinnedResources.spec.ts (1)
63-83: Clean migration — consider removing vestigialasynckeywords.The
renderHookmigration is correctly implemented. However, the test functions are markedasyncbut don't contain anyawaitstatements. This is likely a remnant from the previoustestHookusage. While harmless, removing theasynckeywords would improve clarity.♻️ Optional: Remove unnecessary async
-it('should return default pins from extension if perspectives are not configured', async () => { +it('should return default pins from extension if perspectives are not configured', () => {Apply similarly to all test cases in this file.
frontend/packages/dev-console/src/components/add/hooks/__tests__/useAccessFilterExtensions.spec.ts (1)
27-35: Return hook result and assert viaresult.currentInline assertions inside the render callback can miss updates and make timing brittle. Return the hook result and assert outside (use
waitForif updates are async).♻️ Proposed refactor
- renderHook(() => { - useAddActionsAccessReviewsSpy.mockReturnValue(mockAddAccessReviewResults); - const [filteredAddActionExtensions, loaded] = useAccessFilterExtensions( - namespace, - addActionExtensions, - ); - expect(filteredAddActionExtensions.length).toEqual(0); - expect(loaded).toBe(false); - }); + useAddActionsAccessReviewsSpy.mockReturnValue(mockAddAccessReviewResults); + const { result } = renderHook(() => + useAccessFilterExtensions(namespace, addActionExtensions), + ); + const [filteredAddActionExtensions, loaded] = result.current; + expect(filteredAddActionExtensions.length).toEqual(0); + expect(loaded).toBe(false);- renderHook(() => { - useAddActionsAccessReviewsSpy.mockReturnValue(mockAccessReviewResults); - const [filteredAddActionExtensions, loaded] = useAccessFilterExtensions( - namespace, - addActionExtensions, - ); - expect(filteredAddActionExtensions.length).toEqual(accessAllowedResults.length); - expect(loaded).toBe(true); - }); + useAddActionsAccessReviewsSpy.mockReturnValue(mockAccessReviewResults); + const { result } = renderHook(() => + useAccessFilterExtensions(namespace, addActionExtensions), + ); + const [filteredAddActionExtensions, loaded] = result.current; + expect(filteredAddActionExtensions.length).toEqual(accessAllowedResults.length); + expect(loaded).toBe(true);Also applies to: 48-56
6b56c0b to
8a26f01
Compare
8a26f01 to
f45bbd8
Compare
|
changes only affect unit tests: |
|
@logonoff: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@logonoff The migration from testHook to RTL renderHook looks good! 👍
However, a few files still have assertions inside the renderHook callback. Added comments. Thank you.
frontend/packages/dev-console/src/utils/__tests__/useAddActionExtensions.spec.ts
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/utils/__tests__/useAddActionExtensions.spec.ts
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/utils/__tests__/useAddActionExtensions.spec.ts
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/utils/__tests__/usePerspectiveDetection.spec.ts
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/utils/__tests__/usePerspectiveDetection.spec.ts
Outdated
Show resolved
Hide resolved
...end/packages/webterminal-plugin/src/components/cloud-shell/__tests__/useActivityTick.spec.ts
Outdated
Show resolved
Hide resolved
f29abc0 to
17fff89
Compare
|
/verified by CI |
|
@logonoff: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Refactor all test files to use renderHook from @testing-library/react instead of the deprecated custom testHook utility. This change: - Updates 28 test files across multiple packages to use renderHook - Removes the deprecated hooks-utils.tsx file containing testHook - Fixes tests that had assertions inside hook callbacks to properly use result.current - Removes callback-based patterns (done()) in favor of synchronous assertions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
17fff89 to
5c5ec70
Compare
|
/verified by CI |
|
@logonoff: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@logonoff: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh, logonoff The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Refactor all test files to use renderHook from @testing-library/react instead of the deprecated custom testHook utility. This change:
Summary by CodeRabbit
renderHookacross multiple test suites throughout the codebase. Removed legacy test helper. No changes to application functionality or user experience.✏️ Tip: You can customize this high-level summary in your review settings.